-
Notifications
You must be signed in to change notification settings - Fork 361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a filter for memo and receiver field size #3765
Conversation
…x_memo_size and ics20_max_receiver_size
Regarding the comments on the method validating the fields I ended up changing the logic in b535b44. The method, now
Let me know what you think about this new solution |
Co-authored-by: Romain Ruetschi <romain@informal.systems> Signed-off-by: Luca Joss <43531661+ljoss17@users.noreply.github.com>
Co-authored-by: Romain Ruetschi <romain@informal.systems> Signed-off-by: Luca Joss <43531661+ljoss17@users.noreply.github.com>
Ok((self.build_recv_packet(&event.packet, height)?, None)) | ||
match event | ||
.packet | ||
.are_fields_valid(self.max_memo_size, self.max_receiver_size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we not checking this earlier, i.e. for timeouts also? Same question for build_ack_from_recv_event()
? Maybe even in the supervisor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could put it here https://github.com/informalsystems/hermes/blob/master/crates/relayer/src/link/relay_path.rs#L552 in the match cases:
IbcEvent::TimeoutPacket(_)
IbcEvent::SendPacket(ref event)
IbcEvent::WriteAcknowledgement(ref event)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it more, while it would be nice to have this in the supervisor it would not cover the startup and periodic clearing. So I guess we can check in relay_path
but let's decide if we want to also apply for the other (than recv) message types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check is now performed directly in generate_operational_data
for all events which pertain to an ICS-04 packet: 3d5687e
Left a few comments. In general I am wondering why we don't apply the checks for all msgs that contain a packet, i.e. recv, timeout and ack. |
Good catch, we should indeed do that! Completely slipped my mind… |
Co-authored-by: Romain Ruetschi <romain@informal.systems> Signed-off-by: Luca Joss <43531661+ljoss17@users.noreply.github.com>
I refactored the memo and receiver field configuration and validation in this commit 451b578. I added the option to disable the filter by having the configuration format as following: The reason why I put the I only applied the verification 451b578#diff-072b872f1fbf4ea17384cecde28b35ee339faa9ce969ff9f767647a071da1e36R1405 as I wanted to have your opinion on this solution @ancazamfir @romac. If this seems like a good solution we can think more in detail to where we want to validate these fields |
The check is now performed directly in |
The guide template checker is failing but that's unrelated to this PR. Will fix out of band. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks @ljoss17!
) closes: #10249 ## Description This PR allows ICS-20 fungible token transfer packets to contain inbound receiver and outbound sender addresses formatted as either: - a standard bech32 address (`"agoric1bech32addr"`), or - a base account with optional subpaths and query parameters (`"agoric1bech32addr/opt/account?k=v&k2=v2"`) In these cases, the "base address" is `"agoric1bech32addr"`, and tokens are transferred to that base address. The original address can be parsed by a listening contract into "address parameters" to use at its discretion. ### Security Considerations Should be no impact. If a transfer listener does not explicitly look for the data in an inbound packet, it will behave as if there are no address parameters. Careful review of the code is critical, though, since it touches the powerful `x/vtransfer` module. ### Scaling Considerations Increases the scalability of contracts that would ordinarily have to maintain a pool of multiple Local Chain Account (LCA) addresses to have a unique ID to map to some parameters. Now, contracts can create just one LCA and have the sender supply the data as part of a parameterized address. The ICS-20 transfer addresses are strings limited to at most [2048 bytes, and potentially even smaller if explicitly configured](informalsystems/hermes#3765 (comment)) by a relayer operator. Thus, the total length of the encoded base address and all the address parameters cannot exceed that address string limit, or the packet will not be forwarded. ### Documentation Considerations Should be documented as a feature of `vtransfer`. ### Testing Considerations Unit tests with address parameter data tests have been implemented, testing both the address parser code and the IBC end-to-end packet relay process through the `x/vtransfer` module. ### Upgrade Considerations This is a chain-level, backwards-compatible change, so no upgrade aspects must be addressed. However, Agoric contracts will probably need to import an URL-parsing module to parse URL escaping and query parameters syntax (since the `globalThis.URL` constructor is not currently available in the SwingSet vat environment).
Closes: #3766
Description
This PR adds two packet configurations:
ics20_max_memo_size
which filters packets having a memo field bigger than the configured valueics20_max_receiver_size
which filters packets having a receiver field bigger than the configured valueThese values default to the respective ones in ibc-go v8: https://github.com/cosmos/ibc-go/blob/04e14a7079e18323c64d4e00e12cf77708d24701/modules/apps/transfer/types/msgs.go#L15
PR author checklist:
unclog
.docs/
).Reviewer checklist:
Files changed
in the GitHub PR explorer.